Add support for modbus over serial (RTU)#41
Draft
matthijskooijman wants to merge 3 commits into
Draft
Conversation
This allows a top-level "slave" entry in the config to set the slave address globally (defaulting to 1 as before), but also allows overriding the slave address for individual registers (which allows communicating with multiple slaves on the same bus).
iconnor
reviewed
Jan 23, 2023
| self.prefix = mqtt_topic_prefix | ||
| self.address_offset = self.config.get('address_offset', 0) | ||
| self.registers = self.config['registers'] | ||
| self.default_slave = self.config.get('slave', 1) |
Contributor
There was a problem hiding this comment.
Thanks for making these safer - I like it
iconnor
reviewed
Jan 23, 2023
| slave, addr, value, mask = self._planned_writes.get() | ||
| if mask == 0xFFFF: | ||
| self._mb.write_register(addr, value, unit=0x01) | ||
| self._mb.write_register(addr, value, unit=slave) |
Contributor
There was a problem hiding this comment.
Thank you for fixing up the hard-coded values
Owner
|
@iconnor I've pushed up some commits to a fork of this branch to fix up the tests: master...3devo-serial-modbus I haven't had time to 100% test and finish it off yet though. I also really don't like the "slave" terminology, and I think it's daft that upstream pymodbus is moving from "unit" to "slave". |
|
How does it look like with merge? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR:
I'm marking this as a draft pullrequest, since I do still have some doubts about the implementation:
modbus_interface.__init__()separately. I'm not sure if this approach really scales well, maybe some other method of passing config as a dict or so should be considered.modbus_interface.__init__()has default values for its parameters, but AFAICS these are never used, becausemqtt_interface.connect_modbus()always passes values (and has its own defaults for values missing in the config file).variant, which seems a bit weird to me, but given the only other variant,sungrowis a variation on ModbusTcpClient, it makes sense like this, but does feel a bit weird. Maybe this requires a bit more thought.For now, this code works well enough for my usecase, so I'll probably not put (much) more work in this (I could make some more updates if there is specific feedback), but I wanted to at least share this code in case it is helpful for others as-is already.